Skip to content

Conversation

olamilekan000
Copy link
Contributor

@olamilekan000 olamilekan000 commented Oct 12, 2025

change adds --filter option to list command for a user-friendly way to filter instances using yq expressions,
which is equivalent to yq options like --yq 'select(EXPR)'

Fixes

@olamilekan000 olamilekan000 force-pushed the add-filter-opttion-to-list-cmd branch 2 times, most recently from 4601c97 to c7631d4 Compare October 12, 2025 21:57
}
if len(filter) != 0 {
for _, f := range filter {
yq = append(yq, "select("+f+")")
Copy link
Member

@AkihiroSuda AkihiroSuda Oct 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs escaping


EDIT (2025-10-20): Added an example of bad f:

. | .name=strenv(SOME_SECRET_ENV) | select(.

Copy link
Contributor Author

@olamilekan000 olamilekan000 Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested numerous escaping approaches as asked, but it breaks the yq expressions. What string escape approach would you recommend? Thanks @AkihiroSuda

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda I don't see why this needs escaping. The filter expression should be inserted literally inside the select().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without escaping the code looks vulnerable to "Bobby Tables".
Unlikely to result in breaking data in our case though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different from any code you specify via --yq? The only difference is that the --filter argument is wrapped inside a select() call.

The user can literally send the same command via --yq "$(printf "select(%s)" "$FILTER")".

Any escaping would simply break the filter expression.

Copy link
Member

@jandubois jandubois Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just add a new option in the YQ library to prohibit accessing envs?

I don't know; there doesn't seem to be an easy way to pass settings to the env operator etc.

I agree that this would be a good enhancement (so we could make it apply to all our yqlib calls), but for limactl list it seems like overkill because I don't see an easy way to implement it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda Are you ok with using os.Clearenv() in this particular case (limactl list, just before calling yqlib, printing the result, and exiting)?

I suspect @olamilekan000 is waiting for a decision before addressing the remaining issues, so this would be blocking it from being included in Lima 2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. That's what I am waiting for

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like to approve os.Clearenv() even if it works for the particular case, for long-term maintainability.

Is there any reason that we have to rush to merge this in v2.0?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we just support --format table with --yq?

Originally posted by @AkihiroSuda in #4187 (comment)

--format '{{ <go template> }}' - If the format begins and ends with '{{ }}', then it is used as a go template.
Filtering instances:
--filter EXPR - Filter instances using yq expression (this is equivalent to --yq 'select(EXPR)')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we do not need this flag. We can just add --yq select() examples to --help and call it a day

Copy link
Member

@jandubois jandubois Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I wrote in #4007:

This alone doesn't seem worthwhile, but I would want --filter to also work with --format table and --format '{{GO-TMPL}}'.

So you can do this (which isn't possible with --yq):

cat ~/bin/limactl-ps
#!/bin/sh
limactl ls --filter '.status == "Running"' "$@"l ps
NAME       STATUS     SSH                VMTYPE    ARCH       CPUS    MEMORY    DISK      DIR
docker     Running    127.0.0.1:60763    vz        aarch64    4       4GiB      100GiB    ~/.lima/docker
qemu       Running    127.0.0.1:63754    qemu      aarch64    4       4GiB      100GiB    ~/.lima/qemul ps -f '.config.vmType == "vz"'
NAME       STATUS     SSH                VMTYPE    ARCH       CPUS    MEMORY    DISK      DIR
docker     Running    127.0.0.1:60763    vz        aarch64    4       4GiB      100GiB    ~/.lima/docker

I've not had time yet to review this PR to see if this is properly implemented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we just support --format table with --yq?

Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs changes (allow --filter with --yq) and simplifications.

Comment on lines 162 to 165
if len(yq) != 0 {
return errors.New("option --filter conflicts with option --yq")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should be able to combine --filter and --yq as long as the output format is json or yaml.

So you could just drop the check here because yq is already incompatible with the other output formats.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't too clear to me but is there anything wrong with the way it currently is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes:

l ls -l '.status == "Running"'
NAME          STATUS     SSH                VMTYPE    ARCH       CPUS    MEMORY    DISK      DIR
alpine-iso    Running    127.0.0.1:50496    vz        aarch64    4       4GiB      100GiB    ~/.lima/alpine-isol ls -l '.status == "Running"' --yq .name
FATA[0000] option --filter conflicts with option --yql ls --yq 'select(.status == "Running")' --yq .name
alpine-iso

I expect the last 2 commands to work exactly the same. There is no reason for the len(yq) check to exist.

}
if len(filter) != 0 {
for _, f := range filter {
yq = append(yq, "select("+f+")")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda I don't see why this needs escaping. The filter expression should be inserted literally inside the select().

@olamilekan000 olamilekan000 force-pushed the add-filter-opttion-to-list-cmd branch 3 times, most recently from 404b08c to a5ba9ad Compare October 14, 2025 22:33
Comment on lines 261 to 262
isGoTemplate := strings.HasPrefix(format, "{{") && strings.HasSuffix(format, "}}")
if len(filter) != 0 && (format == "table" || isGoTemplate) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
isGoTemplate := strings.HasPrefix(format, "{{") && strings.HasSuffix(format, "}}")
if len(filter) != 0 && (format == "table" || isGoTemplate) {
if len(filter) != 0 && format != "json" && format != "yaml") {

Despite what the --help output says, every string except table, json, and yaml is treated as a Go template, so the check for the double braces prefix/suffix is not correct:

l ls -f 'Name is: {{.Name}}'
Name is: alpine-iso
Name is: alpine-lima-3.22.2
Name is: foo

for _, instance := range instances {
jsonBytes, err := json.Marshal(instance)
if err != nil {
return nil, fmt.Errorf("failed to marshal instance %s: %w", instance.Name, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, fmt.Errorf("failed to marshal instance %s: %w", instance.Name, err)
return nil, fmt.Errorf("failed to marshal instance %q: %w", instance.Name, err)

Always use %q when printing use input.

Also applies to the next error message.

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olamilekan000 olamilekan000 force-pushed the add-filter-opttion-to-list-cmd branch 2 times, most recently from 224e6e4 to b3f18db Compare October 15, 2025 09:25
Signed-off-by: olalekan odukoya <[email protected]>
@olamilekan000 olamilekan000 force-pushed the add-filter-opttion-to-list-cmd branch from b3f18db to bdef056 Compare October 15, 2025 09:26
@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone Oct 15, 2025
Examples:
--filter '.status == "Running"'
--filter '.vmType == "vz"'
--filter '.status == "Running"' --filter '.vmType == "vz"'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this AND-matching or OR-matching?

Copy link
Member

@jandubois jandubois Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you want the description updated to clarify.

It is AND so you can do this:

cat ~/bin/limactl-ps
#!/bin/sh
limactl ls --filter 'select(.status == "Running")' "$@"l ls
NAME                  STATUS     SSH                VMTYPE    ARCH       CPUS    MEMORY      DISK      DIR
alpine-iso            Running    127.0.0.1:50496    vz        aarch64    4       4GiB        100GiB    ~/.lima/alpine-iso
alpine-lima-3.22.2    Stopped    127.0.0.1:0        vz        aarch64    4       4GiB        100GiB    ~/.lima/alpine-lima-3.22.2
default               Running    127.0.0.1:54512    qemu      aarch64    4       4GiB        100GiB    ~/.lima/default
foo                   Stopped    127.0.0.1:0        vz        aarch64    4       1.177MiB    100GiB    ~/.lima/fool ps
NAME          STATUS     SSH                VMTYPE    ARCH       CPUS    MEMORY    DISK      DIR
alpine-iso    Running    127.0.0.1:50496    vz        aarch64    4       4GiB      100GiB    ~/.lima/alpine-iso
default       Running    127.0.0.1:54512    qemu      aarch64    4       4GiB      100GiB    ~/.lima/defaultl ps -l '.vmType == "vz"'
NAME          STATUS     SSH                VMTYPE    ARCH       CPUS    MEMORY    DISK      DIR
alpine-iso    Running    127.0.0.1:50496    vz        aarch64    4       4GiB      100GiB    ~/.lima/alpine-iso

Comment on lines +257 to +259
if !valid.MatchString(f) {
return fmt.Errorf("unsafe characters in filter expression: %q", f)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is useful. The whole point of --filter and --yq is to provide YQ expressions that are going to be evaluated. There is no vulnerability; the expression is provided by the user themselves.

And we already get an error when the YQ expression is invalid, e.g.

l ls -l '.vmType =='
FATA[0000] failed to apply filter select(.vmType ==): '==' expects 2 args but there is 1

So I don't understand the purpose of this "validation", especially since it might make legitimate use cases invalid (e.g. you are restricting which characters are allowed inside literal strings).

Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The combination of --filter and --quiet is not working (the filter is ignored):

l ls -l '.vmType == "qemu"'
NAME       STATUS     SSH                VMTYPE    ARCH       CPUS    MEMORY    DISK      DIR
default    Running    127.0.0.1:54512    qemu      aarch64    4       4GiB      100GiB    ~/.lima/defaultl ls -l '.vmType == "qemu"' -q
alpine-iso
alpine-lima-3.22.2
default
foo

listCommand.Flags().BoolP("quiet", "q", false, "Only show names")
listCommand.Flags().Bool("all-fields", false, "Show all fields")
listCommand.Flags().StringArray("yq", nil, "Apply yq expression to each instance")
listCommand.Flags().StringArrayP("filter", "l", nil, "Filter instances using yq expression (equivalent to --yq 'select(EXPR)')")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The l shorthand should be probably reserved for --label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants